feat: add failsafe to transaction replay #6212
Conversation
|
After some discussion, we're going to update this to use the rule of "once there are 2 burn blocks past the previous tip, clear the replay set if we're still in it". We'll use a config value for the "2 burn blocks" value. |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## develop #6212 +/- ##
===========================================
+ Coverage 82.45% 82.55% +0.10%
===========================================
Files 541 541
Lines 344244 344309 +65
Branches 323 323
===========================================
+ Hits 283833 284251 +418
+ Misses 60403 60050 -353
Partials 8 8
... and 41 files with indirect coverage changes Continue to review full report in Codecov by Sentry.
🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Overall the implementation looks fine!
As expected, all tx_replay_* tests are currently failing due to the two-tenure limit, for the reasons reported in the PR description. Once the current approach is finalized, these tests will likely need to be revisited and properly adjusted to align with the new logic.
I've included a few remarks throughout the review suggesting possible improvements for readability and maintainability.
This reverts commit c6ca6b9.
github-project-automation
bot
moved this from Status: In Review
to Status: 💻 In Progress
in Stacks Core Eng
|
I've re-requested reviews here - after the latest round of test flakiness fixes from develop, CI is looking good here. |
|
LGTM aside from all the failing tests. I saw similar tests failing in my 2 phase commit PR. maybe take a look there and see if my fixes also fix the issues you are seeing (they are some subtle timing fixes that are probably causing issues on develop but don't always manifest) |
The return type was `Result<bool>`, but it only needed to be `bool`
| pub fn get_chain_tips(config: &Config) -> RPCResult<serde_json::Value> { | ||
| let payload = BitcoinRPCRequest { | ||
| method: "getchaintips".to_string(), | ||
| params: vec![], | ||
| id: "stacks".to_string(), | ||
| jsonrpc: "2.0".to_string(), | ||
| }; | ||
|
|
||
| BitcoinRPCRequest::send(config, payload) | ||
| } | ||
|
|
There was a problem hiding this comment.
Is this method still used?
(Opening as a draft - the test could be improved, and I'm not very confident in the "rules" implemented here)
This PR implements a 'failsafe' for transaction replay - if we've had 2 burn blocks since the new fork tip, clear the replay set. While this is very imperfect, it prioritizes liveness of the chain over guarantees about replay getting executed as expected. Most of the time, this shouldn't make a difference anyways. A new config field,
reset_replay_set_after_fork_blocks, is provided to allow changing this value (which defaults to2).I've also refactored many of the transaction replay tests to do shallower forks, which aligns much more with reality. This actually caught a bug in the fork detection logic, which we were getting away with due to the tests using deeper forks. We now use a descendency check to determine whether a new burn block is a fork, where we previously did a simple check against the height of a new burn block.